[1/3] Graph RIP: refactor+graph: move all graph related DB code to the graph package #9236

ellemouton · 2024-10-29T17:06:57Z

This PR is part of the larger graph separation & gossip updates projects

This PR is mostly a refactor:

(refactor) The first set of commits is a pure refactor that moves all graph related code out of channeldb and into graph/db.
(refactor) Then, it completely removes the ChannelGraph from channeldb.DB. The only place where the two are used together is for AddrSource, so a couple of commits extract this.
(logic change): currently the local channel manager mixes db transactions and passes the same Tx to both calls to the channelDB and the graphDB. So one commit ensures that this is no longer the case.
the final commit (prefixed with TEMP) uses different files/DBs for the graph and channeldb. This is just to demonstrate the clean separation of the two. It is for reviewer confirmation of CI only & will be removed before merge.

Part of #8833

coderabbitai · 2024-10-29T17:07:08Z

Important

Review skipped

Auto reviews are limited to specific labels.

🏷️ Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

yyforyongyu

Thanks for taking the care to make such iterative commits, extremely easy to read and reason about❤️ I think this is almost good to go, just a few questions.

config_builder.go

graph/db/graph.go

channeldb/addr_source_test.go

yyforyongyu · 2024-11-04T07:57:58Z

channeldb/addr_source.go

-	AddrsForNode(nodePub *btcec.PublicKey) ([]net.Addr, error)
+	// key. The returned boolean must indicate if the given node is unknown
+	// to the backing source.
+	AddrsForNode(nodePub *btcec.PublicKey) (bool, []net.Addr, error)


not sure about the last bool since the caller can know if this is known or not by checking the error? But I guess it's nice to have this abstraction so the caller doesn't need to care about the specific errors. Curious about what others think.

On the other hand, if we decide to use the intersection instead of the union of the sets, we can then remove this bool.

since the caller can know if this is known or not by checking the error

Not necessarily since there are possibly multiple backends. The caller should not need to know to check both graphdb.NotFound and channeldb.NotFound.

ut I guess it's nice to have this abstraction so the caller doesn't need to care about the specific errors.

yeah exactly

if we decide to use the intersection instead of the union of the sets, we can then remove this bool.

I think we want the union though for maximum address coverage. The reason we check multiple sources is so that we have a higher probability of using a more up to date address

channeldb/db.go

graph/db/models/node.go

yyforyongyu · 2024-11-04T08:13:57Z

lncfg/db.go

@@ -47,6 +49,8 @@ const (
 	// and channel state DB.
 	NSChannelDB = "channeldb"

+	NSGraphDB = "graphdb"


niiiice - guess we can do a followup PR to implement this and migrations? Think the migration might be huge tho.

i think we might actually not need to ever migrate this since: with the Gossip V2 upgrade, we will start using SQL for new messages & we will start using SQL for legacy channels advertised with the new protocol. So eventually, all the channels in the v1 DB should become zombies as the network migrates to V2 and at that point, we can just delete that DB.

Think that would take years🤔

yeah indeed. so yeah maybe we will end up doing a migration (maybe directly to SQL land) in a follow up.

yyforyongyu · 2024-11-04T08:17:11Z

channeldb/addr_source.go

+
+// AddrsForNode returns all known addresses for the target node public key.
+//
+// NOTE: this implements the AddrSource interface.


could mention that we are using the union of the sets instead of the intersection. And also have a question here - if the node is not found in either DB, does it mean there's a bug somewhere? If it's not in the link db, think the node cannot be used for sending htlcs. when it's not in the graph db, it won't be used for pathfinding.

could mention that we are using the union of the sets instead of the intersection

updated 👍

And also have a question here - if the node is not found in either DB, does it mean there's a bug somewhere? If it's not in the link db, think the node cannot be used for sending htlcs. when it's not in the graph db, it won't be used for pathfinding.

perhaps - but i think that is for the layer making the call to decide.

ellemouton

Thanks @yyforyongyu 🙏 updated!

config_builder.go

graph/db/graph.go

channeldb/addr_source_test.go

ellemouton · 2024-11-07T11:05:14Z

channeldb/addr_source.go

+
+// AddrsForNode returns all known addresses for the target node public key.
+//
+// NOTE: this implements the AddrSource interface.


could mention that we are using the union of the sets instead of the intersection

updated 👍

And also have a question here - if the node is not found in either DB, does it mean there's a bug somewhere? If it's not in the link db, think the node cannot be used for sending htlcs. when it's not in the graph db, it won't be used for pathfinding.

perhaps - but i think that is for the layer making the call to decide.

ellemouton · 2024-11-07T11:08:37Z

channeldb/addr_source.go

-	AddrsForNode(nodePub *btcec.PublicKey) ([]net.Addr, error)
+	// key. The returned boolean must indicate if the given node is unknown
+	// to the backing source.
+	AddrsForNode(nodePub *btcec.PublicKey) (bool, []net.Addr, error)


since the caller can know if this is known or not by checking the error

Not necessarily since there are possibly multiple backends. The caller should not need to know to check both graphdb.NotFound and channeldb.NotFound.

ut I guess it's nice to have this abstraction so the caller doesn't need to care about the specific errors.

yeah exactly

if we decide to use the intersection instead of the union of the sets, we can then remove this bool.

I think we want the union though for maximum address coverage. The reason we check multiple sources is so that we have a higher probability of using a more up to date address

graph/db/models/node.go

ellemouton · 2024-11-07T11:24:39Z

lncfg/db.go

@@ -47,6 +49,8 @@ const (
 	// and channel state DB.
 	NSChannelDB = "channeldb"

+	NSGraphDB = "graphdb"


i think we might actually not need to ever migrate this since: with the Gossip V2 upgrade, we will start using SQL for new messages & we will start using SQL for legacy channels advertised with the new protocol. So eventually, all the channels in the v1 DB should become zombies as the network migrates to V2 and at that point, we can just delete that DB.

channeldb/addr_source_test.go

yyforyongyu · 2024-11-08T07:16:13Z

channeldb/addr_source_test.go

+	}
+
+	addrs, ok := args.Get(0).([]net.Addr)
+	require.True(m.t, ok)


Could we just return args.Get(0).([]net.Addr)?

tried this but the linter didnt like it

i see - think we ignore the forcetypeassert for all the mock files, sth like mocks.go

graph/db/models/node.go

yyforyongyu

I think we just need to assert the mockers and it's good to go🛳️

yyforyongyu · 2024-11-08T07:23:19Z

lncfg/db.go

@@ -47,6 +49,8 @@ const (
 	// and channel state DB.
 	NSChannelDB = "channeldb"

+	NSGraphDB = "graphdb"


Think that would take years🤔

We'll move these to the new graphdb package later and import them from there.

In preparation for moving the graph related schema structs to the graph package in an upcoming commit, we move these methods to the graph package. The structs we will move make use of these methods but we still import them from channeldb so as to keep the ReadElement and WriteElement helpers working as they do today.

We have the same helpers for writing and reading a wire.Outpoint type defined separately in a couple places. We will want to use these from the graph db package soon though so instead of defining them again there, this commit unifies things and creates a single exported set of helpers. The next commit will make use of these.

Start using the single set of exported write/read functions for wire.Outpoint.

All the structs defined in the `channeldb/models` package are graph related. So once we move all the graph CRUD code to the graph package, it makes sense to have the schema structs there too. So this just moves the `models` package over to `graph/db/models`.

This is a pure refactor commit. It moves over all the graph related CRUD code from `channeldb` to `graph/db`.

We also now use the graph DB's own optional functions. An instance of the graph is currently still passed to the channeldb's `CreateWithBackend` function. This will be removed in a later commit once the two have been completely disjoint.

Our aim is to completely remove the `channeldb.DB`'s direct dependence on the `graphdb.ChannelGraph` pointer. The only place where it still depends on this pointer is in the `(DB).AddrsForNode(..)` method where it queries both the channel DB and the graph db for the known addresses for the node in question and then combines the results. So, to separate these out, we will define an AddrsForNodes interface in this commit which we will later let both the ChannelGraph and channeldb.DB both implement and we will merge these results outside of the channeldb package. All this commit does is to unify the `AddrSource` interface since this has been defined separately in a couple of places.

In this commit, we implement a version of the AddrSource interface which merges the results of a set of AddrSource implementations. This will later be used to merge the results of the channel db and graph db.

Before this commit, the `(channeldb.DB).AddrsForNode` method treats the results from the channel db and the graph db slightly differently. It errors out if the channel db is unaware of the node in question but does not error out if the graph is unaware of the node. So this commit changes the logic slightly so that a "node-unknown" error from either backing source is not seen as an error.

Then use both to construct a multiAddrSource AddrSource and use that around the code-base.

Now that the channel.DB no longer uses the ChannelGraph pointer, we can completely remove it as a member of channeldb.DB.

and the same for ChannelStateDB.FetchChannel. Most of the calls to these methods provide a `nil` Tx anyways. The only place that currently provides a non-nil tx is in the `localchans.Manager`. It takes the transaction provided to the `ForAllOutgoingChannels` callback and passes it to it's `updateEdge` method. Note, however, that the `ForAllOutgoingChannels` call is a call to the graph db and the call to `updateEdge` is a call to the `ChannelStateDB`. There is no reason that these two calls need to happen under the same transaction as they are writing to two completely disjoint databases. And so in the effort to completely split untangle the relationship between the two databases, we now dont use the same transaction for these two calls.

So that this fails earlier on if the actual call to UpdateChannelPolicy fails.

Demonstrate CI passes if we persist graph data in a completely separate DB to other channel data.

yyforyongyu

LGTM🦾

yyforyongyu · 2024-11-08T11:13:54Z

channeldb/addr_source_test.go

+	}
+
+	addrs, ok := args.Get(0).([]net.Addr)
+	require.True(m.t, ok)


i see - think we ignore the forcetypeassert for all the mock files, sth like mocks.go

ellemouton force-pushed the moveGraphDBCode branch 6 times, most recently from fee4ca7 to 2b3f659 Compare November 1, 2024 07:19

ellemouton added code health Related to code commenting, refactoring, and other non-behaviour improvements database Related to the database/storage of LND graph refactoring labels Nov 1, 2024

ellemouton force-pushed the moveGraphDBCode branch 4 times, most recently from 8568ef5 to 5ae0c8a Compare November 1, 2024 12:06

yyforyongyu self-requested a review November 1, 2024 12:37

ellemouton force-pushed the moveGraphDBCode branch 6 times, most recently from b465402 to 1504b31 Compare November 3, 2024 10:10

yyforyongyu reviewed Nov 4, 2024

View reviewed changes

yyforyongyu added this to the v0.19.0 milestone Nov 4, 2024

ellemouton mentioned this pull request Nov 4, 2024

[2/3] Graph RIP: multi: Graph Source Abstraction #9243

Open

guggero self-requested a review November 4, 2024 16:24

ellemouton self-assigned this Nov 7, 2024

ellemouton force-pushed the moveGraphDBCode branch 2 times, most recently from 6a99e77 to bc76c5b Compare November 7, 2024 11:45

ellemouton commented Nov 7, 2024

View reviewed changes

ellemouton force-pushed the moveGraphDBCode branch 2 times, most recently from a2d853b to 8885425 Compare November 7, 2024 12:02

ellemouton requested a review from yyforyongyu November 7, 2024 12:52

yyforyongyu reviewed Nov 8, 2024

View reviewed changes

ellemouton added 9 commits November 8, 2024 11:18

channeldb: export net.Addr encode/decode methods

62ab59f

We'll move these to the new graphdb package later and import them from there.

contractcourt: use graphdb outpoint helpers

2a79c7c

Start using the single set of exported write/read functions for wire.Outpoint.

refactor: move graph related DB code to graph/db from channeldb

36eec1d

This is a pure refactor commit. It moves over all the graph related CRUD code from `channeldb` to `graph/db`.

channeldb: implement a multi-source AddrSource

cc4b677

In this commit, we implement a version of the AddrSource interface which merges the results of a set of AddrSource implementations. This will later be used to merge the results of the channel db and graph db.

ellemouton force-pushed the moveGraphDBCode branch from 8885425 to a066df1 Compare November 8, 2024 09:23

ellemouton added 10 commits November 8, 2024 11:23

multi: let chan and graph db implement AddrSource

829d50e

Then use both to construct a multiAddrSource AddrSource and use that around the code-base.

channeldb: remove graph db from channeldb

abfad5b

Now that the channel.DB no longer uses the ChannelGraph pointer, we can completely remove it as a member of channeldb.DB.

refactor: move graphsession pkg to graph package

adeaf15

multi: move LightningNode struct to models package

9431edb

itest: assert no failed updates in test

7edf99f

So that this fails earlier on if the actual call to UpdateChannelPolicy fails.

multi: fix linter errors

fb6c6ba

docs: add release notes entry for 9236

aa92e4d

TEMP: show diff files separation works

e017ccf

Demonstrate CI passes if we persist graph data in a completely separate DB to other channel data.

ellemouton force-pushed the moveGraphDBCode branch from a066df1 to e017ccf Compare November 8, 2024 09:23

ellemouton requested a review from yyforyongyu November 8, 2024 09:32

yyforyongyu approved these changes Nov 8, 2024

View reviewed changes

ellemouton changed the title ~~refactor+graph: move all graph related DB code to the graph package~~ [1/3] Graph RIP: refactor+graph: move all graph related DB code to the graph package Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/3] Graph RIP: refactor+graph: move all graph related DB code to the graph package #9236

[1/3] Graph RIP: refactor+graph: move all graph related DB code to the graph package #9236

ellemouton commented Oct 29, 2024 •

edited

Loading

coderabbitai bot commented Oct 29, 2024

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

yyforyongyu left a comment

yyforyongyu Nov 4, 2024

ellemouton Nov 7, 2024

yyforyongyu Nov 4, 2024

ellemouton Nov 7, 2024

yyforyongyu Nov 8, 2024

ellemouton Nov 8, 2024

yyforyongyu Nov 4, 2024

ellemouton Nov 7, 2024

ellemouton left a comment

ellemouton Nov 7, 2024

ellemouton Nov 7, 2024

ellemouton Nov 7, 2024

yyforyongyu Nov 8, 2024

ellemouton Nov 8, 2024

yyforyongyu Nov 8, 2024

yyforyongyu left a comment

yyforyongyu Nov 8, 2024

yyforyongyu left a comment

yyforyongyu Nov 8, 2024

[1/3] Graph RIP: refactor+graph: move all graph related DB code to the graph package #9236

Are you sure you want to change the base?

[1/3] Graph RIP: refactor+graph: move all graph related DB code to the graph package #9236

Conversation

ellemouton commented Oct 29, 2024 • edited Loading

coderabbitai bot commented Oct 29, 2024

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton commented Oct 29, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)